-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
enh: (helm) - add annotation to wait for all resources to be deleted before removing CR finalizer #4487
enh: (helm) - add annotation to wait for all resources to be deleted before removing CR finalizer #4487
Conversation
…before removing finalizer Signed-off-by: Mike Ng <ming@redhat.com>
For Helm based-operators, added annotation `helm.sdk.operatorframework.io/uninstall-wait: "true"` | ||
to allow all resources to be deleted before removing the custom resource's finalizer. | ||
kind: "addition" | ||
breaking: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit : space at the end of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Done as suggested.
_ = r.updateResourceStatus(ctx, o, status) | ||
return reconcile.Result{}, err | ||
} | ||
if !areAllResDeleted { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if !areAllResDeleted { | |
if !isAllResourcesDeleted { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Done as suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is very completed. It is a great work has docs, changelogs and tests 🥇
I am ok with the change. Just added a nit suggestion.
However, would be nice check with @joelanford or any other team member before we add this one.
Otherwise /lgtm
status.RemoveCondition(types.ConditionReleaseFailed) | ||
} | ||
|
||
log.Info("Removing finalizer") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Signed-off-by: Mike Ng <ming@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, just one potential bugfix
…ction Signed-off-by: Mike Ng <ming@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mikeshng!
/lgtm
Signed-off-by: Mike Ng ming@redhat.com
Description of the change:
For Helm based-operators, added annotation
helm.sdk.operatorframework.io/uninstall-wait: "true"
option to allow all resources to be deleted before removing the custom resource's finalizer.Motivation for the change:
Closes #4401
Checklist
If the pull request includes user-facing changes, extra documentation is required:
changelog/fragments
(seechangelog/fragments/00-template.yaml
)website/content/en/docs